Skip to content

Conversation

@semanser
Copy link

@semanser semanser commented Mar 7, 2018

This is extremely useful if we have multiple DOM element with the same class name. E.g:

const steps = {
        element: document.querySelectorAll(`.example`)[0],
        intro: 'Intro here',
      },
      {
        element: document.querySelectorAll(`.example`)[1],
        intro: 'Intro here,
      }

@coveralls
Copy link

coveralls commented Mar 7, 2018

Coverage Status

Coverage remained the same at 100.0% when pulling 7d0b3ca on semanser:master into 9a9fe1f on HiDeoo:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.2%) to 98.83% when pulling 10e59cf on semanser:master into 9a9fe1f on HiDeoo:master.

@semanser semanser changed the title Element parameter now can be plain DOM element Element parameter of Steps now can be plain DOM element Mar 7, 2018
@HiDeoo
Copy link
Owner

HiDeoo commented Mar 7, 2018

Hi, thanks for your PR 😄

I didn't take a look or tested the code yet but that seems to be a reasonable change.

I'm a little bit confused however on the updateStepElement changes. The most useful case for this method is when the associated element is not present in the DOM when initialized. If the element is declared using document.querySelectorAll('.example')[0] it'll evaluate immediately to undefined so even when triggering updateStepElement it'll still be undefined.

I may be missing something as I previously said, but if I'm not, maybe it could be worth to consider a refactor of updateStepElement accepting a second argument to redefine the element entirely using either a new string or a DOM element.

@semanser
Copy link
Author

semanser commented Mar 7, 2018

Ok, I will take a closer look at about what you did you say.
I am using this in my project, but yes, now I am not sure if it will be working for non-initialized elements.

@HiDeoo
Copy link
Owner

HiDeoo commented May 8, 2018

Closing for inactivity, feel free to re-open if needed.

@HiDeoo HiDeoo closed this May 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants